Conversation
Jaytee-fn
force-pushed
the
jayarora/FnCLI
branch
from
885deda to
6e96ab1
Compare
Jaytee-fn
force-pushed
the
jayarora/FnCLI
branch
from
39e2d65 to
fae33f2
Compare
| return | ||
| var hostedPlatform = runtime.GOARCH | ||
| if platform, ok := TargetPlatformMap[shape]; ok { | ||
| targetPlatform := strings.Join(platform," ") |
There was a problem hiding this comment.
Why are we appending " " to platform?
There was a problem hiding this comment.
Added comment in the code
common/common.go
Outdated
| targetPlatform := strings.Join(platform," ") | ||
| if targetPlatform != hostedPlatform { | ||
| fmt.Println("TargetedPlatform and hostPlatform are not same") | ||
| if containerEngineType == ContainerEngineType { |
There was a problem hiding this comment.
Change the variable name to reflect docker type.
Please move this condition to the method
Jaytee-fn
force-pushed
the
jayarora/FnCLI
branch
2 times, most recently
from
24c1515 to
b64346d
Compare
Jaytee-fn
force-pushed
the
jayarora/FnCLI
branch
from
b64346d to
31dcfc0
Compare
sudhindraaoracle
left a comment
sudhindraaoracle
left a comment
There was a problem hiding this comment.
Optimise the logs and format them properly. Logic seems to be fine.
| if !p.local { | ||
| // fetch the architectures | ||
| shape = app.Shape | ||
| fmt.Printf("*****shape is %v *******", shape) |
There was a problem hiding this comment.
Can we print this log once the shape is properly assigned (after default case ie., line 400)?
| buildCommand, | ||
| "build", | ||
| "-t", name, | ||
| //"-t", name, |
| } | ||
|
|
||
| if containerEngineType != containerEngineTypeDocker { | ||
| fmt.Println("*** engine type not docker append load") |
There was a problem hiding this comment.
Can these be in 'DEBUG' logs? It will be too 'internal' for the INFO logs.
| var buildCommand = "buildx" | ||
| var name = imageName | ||
|
|
||
| plat:= strings.Join(architectures,"," ) |
There was a problem hiding this comment.
plat is not comprehensive. You can directly use 'strings.Join' command in fmt.Println()
Another variable is not required.
Buildx platform confusing. You can say 'Specified Platform/s : '
| func buildDockerCommand(imageName, dockerfile string, buildArgs []string, noCache bool, architectures []string) []string { | ||
| var name = imageName | ||
| plat:= strings.Join(architectures,"," ) | ||
| fmt.Println("build platform is %v", plat) |
There was a problem hiding this comment.
plat is not comprehensive. You can directly use 'strings.Join' command in fmt.Println()
Another variable is not required.
Build platform confusing. You can say 'Specified Platform/s : '
There was a problem hiding this comment.
Make the logs look more professional
| if platform, ok := TargetPlatformMap[shape]; ok { | ||
| // create target platform string to compare with hosted platform | ||
| targetPlatform := strings.Join(platform," ") | ||
| fmt.Println("hosted platform %v target platform %v", hostedPlatform, targetPlatform) |
| targetPlatform := strings.Join(platform," ") | ||
| fmt.Println("hosted platform %v target platform %v", hostedPlatform, targetPlatform) | ||
| if targetPlatform != hostedPlatform { | ||
| fmt.Println("TargetedPlatform and hostPlatform are not same") |
There was a problem hiding this comment.
This can be inferred from the log at line 549. Do we need it again?
As the logs are public facing, lets limit the INFO logs. If its needed for DEBUG, lets put it in DEBUG logs
| defer cleanupContainerBuilder(containerEngineType) | ||
| } else { | ||
| fmt.Println("TargetedPlatform and hostPlatform are same") | ||
| fmt.Println("1. issuePush is ", issuePush) |
There was a problem hiding this comment.
Do we need these issuePush logs?
| dockerBuildCmdArgs = buildDockerCommand(imageName, dockerfile, buildArgs, noCache) | ||
| } | ||
|
|
||
| fmt.Println("*****containerEngineType*****",containerEngineType ) |
There was a problem hiding this comment.
Can we merge it into a single log?
| return fmt.Errorf("build cancelled on signal %v", signal) | ||
| } | ||
| if containerEngineType != containerEngineTypeDocker || issuePush { | ||
| fmt.Println("Using Container engine", containerEngineType, "to push as --push id only for docker and --load is for podman") |
There was a problem hiding this comment.
Optimise these logs.
Log statement need not be true because we use push even for podman with Build command
2 participants
No description provided.